exporter/containerimage: default to oci-mediatypes=true #6095
Conversation
This helper has not been used for some time. Signed-off-by: Bjorn Neergaard <[email protected]>
To facilitate this change, we now fail when there is an exporter option conflict instead of implicitly setting oci-mediatypes=true. Signed-off-by: Bjorn Neergaard <[email protected]>
Signed-off-by: Bjorn Neergaard <[email protected]>
This matches the documentation, which never specified behavior for empty/unknown (non-boolean) strings. We can also remove the parseBoolWithDefault helper, as this was the last consumer. Signed-off-by: Bjorn Neergaard <[email protected]>
Signed-off-by: Bjorn Neergaard <[email protected]>
| RefCfg: cacheconfig.RefConfig{ | ||
| Compression: compression.New(compression.Default), | ||
| }, | ||
| OCITypes: true, |
There was a problem hiding this comment.
Should we start considering changing the defaults in code? (Have a "DisableOCITypes" boolean, and start sunsetting the "OCITypes" bool)?
There was a problem hiding this comment.
We discussed this a bit; @tonistiigi expressed a preference for keeping the exporter as-is for now, and just initializing the instance with the modified default.
neersighted
changed the title
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
@neersighted Check the CI errors. Some tests expecting different mediatype and linter.
| case exptypes.OptKeyName: | ||
| c.ImageName = v | ||
| case exptypes.OptKeyOCITypes: | ||
| err = parseBoolWithDefault(&c.OCITypes, k, v, true) |
There was a problem hiding this comment.
Not handling the empty value in here seems like possibly breaking change. Don't remember the history behind it but maybe don't break it as it seems harmeless.
There was a problem hiding this comment.
That change was made for the other types that originally used this parsing function, so based on that I'm relatively confident in/in favor of this change.
When support for zstd was introduced, these mediaTypes were not yet available in released versions of their respective Go packages. That is no longer the case. Signed-off-by: Bjorn Neergaard <[email protected]>
Yeah, I'm having issues getting the tests to run locally on arm64 due to platform mismatches in the Dockerfile linter tests throwing up a LOT of noise. A lot of tests have implicit assumptions about this behavior, and I will work through them and make them pass (and add a regression test to ensure this behavior is as intended in the future) once I have a little more time to review the CI runs. Unless you have some tips on how to get the tests suite to pass (on master) locally on an arm64 mac? |
|
Just leaving a note that I still mean to get back to this and get the tests added/passing; but I've been having to deal with other concerns at work, and I'll be off for a few days. I'm going to block out some time to get this over the finish line when I get back. |
when we were pushing these before, the registry was not getting their manifests properly. exporting these with `OCI-mediatypes` true produces a good manifest. this will eventually be the default, see moby/buildkit#6171 and moby/buildkit#6095
3 participants
The change to provenance-by-default implicitly changed the default mediaTypes to OCI, but an explicit
--provenance=falsenegates this and causes surprising behavior.To fix this, we make the explicit default
oci-mediatypes=trueand makeoci-mediatypes=falsefail for incompatible cases.Addresses #6094.
Tests are needed/missing; I've had some issues getting the tests working locally and skipped them for now.